Make RequestsCookieJar.popitem() work#7513
Conversation
RequestsCookieJar subclasses both CookieJar and MutableMapping. Its __iter__ comes from CookieJar and yields Cookie objects rather than names, so the popitem() inherited from MutableMapping did `self[next(iter(self))]`, i.e. looked a cookie up by a Cookie object, and always raised KeyError even when the jar was non-empty. Override popitem() to remove and return a (name, value) pair using the jar's dict-like view, and raise KeyError only when the jar is empty, matching the documented behavior.
avinashkamat48
left a comment
avinashkamat48
left a comment
There was a problem hiding this comment.
popitem() removes by cookie name only (del self[name]) after reading the first (name, value) pair. That can remove more than the single cookie returned when the jar contains multiple cookies with the same name on different domains/paths, or hit the existing duplicate-name conflict behavior. Please add a test with two cookies sharing a name but differing by domain/path and remove the exact cookie object selected by the iterator rather than deleting by name alone.
popitem() read the first (name, value) pair from the iterator but then removed the cookie with `del self[name]`, which deletes every cookie sharing that name across other domains or paths. With two cookies of the same name on different domains, that dropped both even though popitem() reported removing only one. Select the cookie via the iterator and remove that exact object with clear(domain, path, name), so only the returned cookie is discarded. Add a regression test covering two same-named cookies on different domains.
|
Good catch, fixed. popitem() now selects the cookie via the iterator and removes that exact object with |
2 participants
Closes #6190
RequestsCookieJarsubclasses bothCookieJarandMutableMapping[str, str | None]. Its__iter__is inherited fromCookieJarand yieldsCookieobjects rather than names. Thepopitem()it inherited fromMutableMappingdoes roughly:so
popitem()always raisedKeyError, even on a non-empty jar:This overrides
popitem()to remove and return a(name, value)pair using the jar's existing dict-like view (iteritems()), and to raiseKeyErroronly when the jar is empty, matching the documented behavior and theMutableMapping[str, str | None]contract.Added a regression test (
test_cookie_popitem) that pops both entries, checks each returned pair is a real(name, value)from the jar, and confirms aKeyErroron the empty jar. It fails onmainand passes with this change.